Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flake8-bugbear] Implement loop-iterator-mutation (B909) #9578

Merged
merged 18 commits into from
Apr 11, 2024

Conversation

mimre25
Copy link
Contributor

@mimre25 mimre25 commented Jan 19, 2024

Summary

This PR adds the implementation for the current flake8-bugbear's B038 rule.
The B038 rule checks for mutation of loop iterators in the body of a for loop and alerts when found.

Rational:
Editing the loop iterator can lead to undesired behavior and is probably a bug in most cases.

Closes #9511.

Note there will be a second iteration of B038 implemented in flake8-bugbear soon, and this PR currently only implements the weakest form of the rule.
I'd be happy to also implement the further improvements to B038 here in ruff 🙂
See PyCQA/flake8-bugbear#454 for more information on the planned improvements.

Test Plan

Re-using the same test file that I've used for flake8-bugbear, which is included in this PR (look for the B038.py file).

Note: this is my first time using rust (beside rustlings) - I'd be very happy about thorough feedback on what I could've done better 🙂 - Bring it on 😀

The B038 rule checks for mutation of loop iterators in the body
of a for loop and alerts when found.
Editing the loop iterator can lead to undesired behavior and is probably
a bug in most cases.
@charliermarsh charliermarsh self-requested a review January 19, 2024 16:15
@charliermarsh charliermarsh added rule Implementing or modifying a lint rule preview Related to preview mode features labels Jan 19, 2024
@charliermarsh
Copy link
Member

Awesome, thank you! Excited to review, I'll give it a close read per request :)

return _to_name_str(func);
}
_ => {
return "".into();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels off to me... Can you give me some more context on what this function is intended to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is just the base case of the recursion.

Probably gonna remove this function anyway in favor of compose_call_path.

}
}
Expr::Call(ExprCall { range: _, func, .. }) => {
return _to_name_str(func);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that foo().bar and foo.bar would be considered identical?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried out collect_call_path and compose_call_path as you've suggested below, and both of them return None for a().foo.

Not 100% sure how I should go about this - should this be the case?

Also wondering, would you say the following should be caught?:

for bar in a().foo:
  del a().foo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say that should not be caught, since we can't know that a() returns the same object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, then I think I can use compose_call_path - I was just worrying that it returns None whenever there is a() in the call path.

}
}

fn _to_name_str(node: &Expr) -> String {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want collect_call_path for this, we have an abstraction for this exact thing, but it stores structured components (like a vector of the dot-separated pieces) rather than a String.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be that the function was removed or renamed? I can't find it anymore.

@charliermarsh
Copy link
Member

Thank you! Left some comments -- feel free to ask questions.

@mimre25
Copy link
Contributor Author

mimre25 commented Jan 25, 2024

Do you have any suggestion on how to set up the work environment?

In general I think rust's incremental builds are a good idea, but damn they eat up like 10+gb for this project 😬

@charliermarsh
Copy link
Member

Awesome, I should be able to review and merge this today!

@mimre25
Copy link
Contributor Author

mimre25 commented Jan 25, 2024

@charliermarsh please hold off from merging, I still want to remote the to_name_str function and use collect_call_path instead.

Also, in flake8-bugbear there is currently the motion going on to make this an optional rule (disabled per default) as there are some false positives stemming from the python standard library - See the discussion in PyCQA/flake8-bugbear#455

@charliermarsh
Copy link
Member

@mimre25 - Roger that!

@charliermarsh charliermarsh marked this pull request as draft January 25, 2024 17:30
@charliermarsh
Copy link
Member

@mimre25 - I'm going to convert to draft for now just to help manage our review workflow.

@MichaReiser
Copy link
Member

I close because it is stale. Please re-open if you plan to follow up on the remaining work.

@mimre25
Copy link
Contributor Author

mimre25 commented Apr 11, 2024

Alright, I finally got around to finish this.

This is now on parity with flake8-bugbear's B909 (it was renamed).

Main new features in the new commits are described in PyCQA/flake8-bugbear#454.
Further, the rule now allows mutations before an unconditional break.

@mimre25 mimre25 marked this pull request as ready for review April 11, 2024 15:16
@charliermarsh charliermarsh self-assigned this Apr 11, 2024
@charliermarsh
Copy link
Member

Thanks! I'll take a look.

@charliermarsh charliermarsh changed the title feat(rules): Implement flake8-bugbear B038 rule [flake8-bugbear] Implement B909 Apr 11, 2024
@charliermarsh charliermarsh changed the title [flake8-bugbear] Implement B909 [flake8-bugbear] Implement loop-iterator-mutation (B909) Apr 11, 2024
}

/// Handle, e.g., `items += [1]`.
fn handle_aug_assign(&mut self, range: TextRange, target: &Expr) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these &Box<Expr> were changed to &Expr.

}

/// Handle, e.g., `items[0] = 1`.
fn handle_assign(&mut self, range: TextRange, targets: &[Expr]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these &Vec<Expr> were changed to &[Expr]. You almost always want the latter -- it's a lot more flexible, because any reference to a vector can be passed in, but so can other kinds of slices.

Copy link
Contributor

github-actions bot commented Apr 11, 2024

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check encountered linter errors. (no lint changes; 1 project error)

pypa/setuptools (error)

Cannot overwrite a value (at line 26, column 2)

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+5 -0 violations, +0 -0 fixes in 3 projects; 1 project error; 40 projects unchanged)

apache/airflow (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ airflow/models/taskinstance.py:3758:17: B909 Mutation to loop iterable `new_dict` during iteration
+ airflow/providers/openlineage/sqlparser.py:353:13: B909 Mutation to loop iterable `tables` during iteration

python/mypy (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ mypy/build.py:3200:21: B909 Mutation to loop iterable `new` during iteration

scikit-build/scikit-build (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ tests/pytest_helpers.py:49:13: B909 Mutation to loop iterable `expected_content` during iteration
+ tests/pytest_helpers.py:92:13: B909 Mutation to loop iterable `expected_content` during iteration

pypa/setuptools (error)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

Cannot overwrite a value (at line 26, column 2)

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
B909 5 5 0 0 0

@charliermarsh
Copy link
Member

There are some false positives in here. For example:

for auth_method in authentication_methods_dict:
    authentication_methods_dict[auth_method] = True

That's probably the most common?

@charliermarsh charliermarsh enabled auto-merge (squash) April 11, 2024 19:45
@charliermarsh charliermarsh merged commit 03899dc into astral-sh:main Apr 11, 2024
17 checks passed
Glyphack pushed a commit to Glyphack/ruff that referenced this pull request Apr 12, 2024
…l-sh#9578)

## Summary
This PR adds the implementation for the current
[flake8-bugbear](https://github.com/PyCQA/flake8-bugbear)'s B038 rule.
The B038 rule checks for mutation of loop iterators in the body of a for
loop and alerts when found.

Rational: 
Editing the loop iterator can lead to undesired behavior and is probably
a bug in most cases.

Closes astral-sh#9511.

Note there will be a second iteration of B038 implemented in
`flake8-bugbear` soon, and this PR currently only implements the weakest
form of the rule.
I'd be happy to also implement the further improvements to B038 here in
ruff 🙂
See PyCQA/flake8-bugbear#454 for more
information on the planned improvements.

## Test Plan
Re-using the same test file that I've used for `flake8-bugbear`, which
is included in this PR (look for the `B038.py` file).


Note: this is my first time using `rust` (beside `rustlings`) - I'd be
very happy about thorough feedback on what I could've done better
:slightly_smiling_face: - Bring it on :grinning:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule: Flake8-bugbear B038 " detect changes to iterable object of loop"
4 participants